Igor/bug fixes refactors by igor47 · Pull Request #58 · igor47/csheet

igor47 · 2025-11-21T18:20:44Z

mostly bug fixes:

allow reed to do rule clarification
fix some missing updates on item changes
correct lightbox crop
correct sorting by ulid, esp. on character list view
fixed damage editing for weapon items
fixed lots of incorrectly-defined notes fields
consistent ids for forms and elements
consistently use morphing and submit event syncing in forms

we do have two new features:

item effects tool for Reed
a more attractive list for characters page

in case there's an effects change which affects how spell stuff is calculated

cropping is just for avatar display

prevents incorrect level display on characters page

makes sure our ulids are as-often as-possible monotonically increasing

they're sortable, like dates, always indexed, and we at least try to make sure they're monotonically increasing (when generated together on the same worker, which is when we are likeliest to get collisions)

vs a boring table.

button was not working

mostly affects notes fields

Copilot

Pull request overview

This PR addresses bug fixes and refactoring across the codebase with a focus on improving consistency and fixing ULID-based sorting issues. The main changes include:

Purpose: Fix multiple bugs related to sorting, form IDs, item state updates, and avatar cropping. Introduces item effects tools for the AI assistant (Reed) and improves the character list UI.

Key Changes:

Centralized ULID generation to @src/lib/ids for consistent monotonic ID generation
Fixed avatar cropper to properly handle image padding in both dimensions
Updated database queries to sort by id (ULID) instead of created_at for reliable chronological ordering
Added AI tools for creating and deleting item effects
Refactored character list from table to card-based grid with avatar display
Standardized form patterns using new ModalForm component

Reviewed changes

Copilot reviewed 61 out of 61 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`static/avatar-cropper.js`	Fixed vertical padding calculations for proper crop coordinate conversion
`src/lib/ids.ts`	Centralized monotonic ULID factory
`src/db/*.ts`	Updated imports to use centralized ULID, replaced `created_at` sorting with `id` sorting
`src/services/listCharacters.ts`	Added avatar data to character list with proper ULID-based sorting
`src/services/*ItemEffect.ts`	Added AI tool wrappers for item effect management
`src/tools.ts`	Registered new item effect tools
`src/routes/character.tsx`	Added missing `SpellsPanel` OOB swaps for item state changes
`src/components/ModalForm.tsx`	New reusable component for consistent form patterns
`src/components/*Form.tsx`	Refactored to use `ModalForm` with consistent ID naming
`src/components/Characters.tsx`	Converted table layout to card grid with avatars
`src/test/factories/item.ts`	New factory for test item creation
`src/routes/character.test.ts`	Updated test assertion for title attribute

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/components/EditItemForm.tsx

src/components/CreateItemForm.tsx

Copilot · 2025-11-21T18:22:56Z

static/avatar-cropper.js

      const cropX = cropForm.querySelector('input[name="crop_x_percent"]');
      const cropY = cropForm.querySelector('input[name="crop_y_percent"]');
      const cropW = cropForm.querySelector('input[name="crop_width_percent"]');
      const cropH = cropForm.querySelector('input[name="crop_height_percent"]');


The form field IDs have been updated to use hyphens (e.g., crop-x-percent), but the JavaScript code still references the old underscore names (e.g., crop_x_percent). The querySelector calls will fail to find these elements.

Update the JavaScript to use the new hyphenated IDs:

const cropX = cropForm.querySelector('input[id="crop-x-percent"]'); const cropY = cropForm.querySelector('input[id="crop-y-percent"]'); const cropW = cropForm.querySelector('input[id="crop-width-percent"]'); const cropH = cropForm.querySelector('input[id="crop-height-percent"]');

src/components/AvatarCropper.tsx

we prefer `-` to `_`. we fix some mismatches between labels and elements

if it exceeds them, we snap back to the bounds, not just prevent the event default.

allows us to manage the common htmx attributes in one spot

we try to make submit take precedence over validation, though we don't always succeed

Copilot

Pull request overview

Copilot reviewed 61 out of 61 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

src/components/CharacterImport.tsx:157

Missing id attribute on the input element. The label references charimport-ability-${ability} but the input doesn't have any id attribute set, breaking the label-input association.

Add id={charimport-ability-${ability}} to the input element.

              <div class="input-group">
                <input
                  type="number"
                  class={clsx("form-control", { "is-invalid": hasError })}
                  id={`charimport-ability-${ability}`}
                  name={`ability_${ability}`}
                  value={score}
                  min="3"
                  max="20"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-21T18:53:10Z

src/components/EditItemForm.tsx

+                      <label class="form-check-label small" for={`edititem-damage-versatile-${i}`}>
+                        Versatile


The label's for attribute references damage-versatile-${i} but the input's id is edititem-damage-versatile-${i}. This mismatch will break the label-input association.

Update the label's for attribute to match: for={edititem-damage-versatile-${i}}

Copilot · 2025-11-21T18:53:11Z

src/components/AvatarCropper.tsx

+              data-existingx={existingCrop.x ?? null}
+              data-existingy={existingCrop.y ?? null}
+              data-existingw={existingCrop.width ?? null}
+              data-existingh={existingCrop.height ?? null}


The data-existing* attributes are set to null when values are not present, but JavaScript's dataset API converts null to the string "null". This means the condition existingx != null on line 136 of avatar-cropper.js will always be true (even when attributes are "null"), breaking the logic.

Change ?? null to ?? undefined for all four data attributes, as undefined won't render the attribute at all, which is the correct behavior.

Suggested change

data-existingx={existingCrop.x ?? null}

data-existingy={existingCrop.y ?? null}

data-existingw={existingCrop.width ?? null}

data-existingh={existingCrop.height ?? null}

data-existingx={existingCrop.x ?? undefined}

data-existingy={existingCrop.y ?? undefined}

data-existingw={existingCrop.width ?? undefined}

data-existingh={existingCrop.height ?? undefined}

igor47 added 10 commits November 15, 2025 15:21

fix: prompt is clear that DnD rules advice is okay

6cff6f0

feat: tools for creating/deleting item effects

2c937c7

fix: wearing/removing/wielding items should update spells panel

49c6bd6

in case there's an effects change which affects how spell stuff is calculated

fix: lightbox shows entire image (uncropped)

851a1c7

cropping is just for avatar display

fix: sort levels by id

1f6aa3b

prevents incorrect level display on characters page

feat: use ulid monotonic factory

9cbfcca

makes sure our ulids are as-often as-possible monotonically increasing

refactor: sort by ids in preference to created_at times

72fba0d

they're sortable, like dates, always indexed, and we at least try to make sure they're monotonically increasing (when generated together on the same worker, which is when we are likeliest to get collisions)

feat: characters list is a card grid

f1636cc

vs a boring table.

fix: add damage rows to edit item form

b899ef5

button was not working

fix: correct textarea values

88390b4

mostly affects notes fields

igor47 requested a review from Copilot November 21, 2025 18:20

Copilot started reviewing on behalf of igor47 November 21, 2025 18:21 View session

Copilot finished reviewing on behalf of igor47 November 21, 2025 18:22

Copilot AI reviewed Nov 21, 2025

View reviewed changes

igor47 added 2 commits November 21, 2025 10:45

refactor: consistent ids in forms

1d92c33

we prefer `-` to `_`. we fix some mismatches between labels and elements

fix: make sure avatar cropper stays within image bounds

94199e6

if it exceeds them, we snap back to the bounds, not just prevent the event default.

igor47 force-pushed the igor/bug-fixes-refactors branch from d0ec337 to 3acc4f3 Compare November 21, 2025 18:49

igor47 added 2 commits November 21, 2025 10:50

refactor: common components for modal form and submit

9836c49

allows us to manage the common htmx attributes in one spot

feat: all forms use morphing and sync

2e63121

we try to make submit take precedence over validation, though we don't always succeed

igor47 force-pushed the igor/bug-fixes-refactors branch from 3acc4f3 to 2e63121 Compare November 21, 2025 18:50

igor47 requested a review from Copilot November 21, 2025 18:50

Copilot started reviewing on behalf of igor47 November 21, 2025 18:51 View session

Copilot finished reviewing on behalf of igor47 November 21, 2025 18:52

Copilot AI reviewed Nov 21, 2025

View reviewed changes

igor47 merged commit 3893871 into main Nov 21, 2025
8 checks passed

igor47 deleted the igor/bug-fixes-refactors branch November 21, 2025 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Igor/bug fixes refactors#58

Igor/bug fixes refactors#58
igor47 merged 14 commits intomainfrom
igor/bug-fixes-refactors

igor47 commented Nov 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Copilot AI Nov 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		<label class="form-check-label small" for={`edititem-damage-versatile-${i}`}>
		Versatile

Conversation

igor47 commented Nov 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants